[PM-34009] chore: Create KeychainServiceFacade by KatherineInCode · Pull Request #2507 · bitwarden/ios

KatherineInCode · 2026-03-30T20:29:42Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-34009

📔 Objective

This consolidates logic that was spread between BitwardenShared.KeychainRepository, AuthenticatorShared.KeychainRepository, and AuthenticatorBridgeKit.KeychainStorage into one KeychainServiceFacade object, and updates the KeychainRepository objects accordingly. In the process, I also tried to write new tests in Swift Testing, and convert a lot of our bespoke mock objects into AutoMockable, and therefore also needed to update tests.

This effectively updates the architecture from this:

flowchart TD
    BSKRAT[getAccessToken]
    BSKRNL[getNeverLock]
    BSKRG[getValue]

    subgraph BitwardenShared.KeychainRepository
    direction TD
    BSKRAT-->BSKRG
    BSKRNL-->BSKRG
    end

    ASKRSK[getSecretKey]
    ASKRUAK[getUserAccessKey]
    ASKRG[getValue]
    subgraph AuthenticatorShared.KeychainRepository
    direction TD
    ASKRSK-->ASKRG
    ASKRUAK-->ASKRG
    end
    
    subgraph AuthenticatorBridgeKit.KeychainRepository
    direction TD
    ABKAK[getAuthenticatorKey]
    ABKALT[getAutoLogoutTime]
    end
    subgraph AuthenticatorBridgeKit.KeychainStorage
    ABKG[getValue]
    ABKAK-->ABKG
    ABKALT-->ABKG
    end

    subgraph BitwardenKit.KeychainService
    BKKSS[search]
    BSKRG-->BKKSS
    ASKRG-->BKKSS
    ABKG-->BKKSS
    end

To this:

flowchart TD
    subgraph BitwardenShared.KeychainRepository
    direction TD
    BSKRAT[getAccessToken]
    BSKRNL[getNeverLock]
    end

    subgraph AuthenticatorShared.KeychainRepository
    direction TD
    ASKRSK[getSecretKey]
    ASKRUAK[getUserAccessKey]
    end
    
    subgraph AuthenticatorBridgeKit.KeychainRepository
    direction TD
    ABKAK[getAuthenticatorKey]
    ABKALT[getAutoLogoutTime]
    end

    subgraph BitwardenKit.KeychainServiceFacade
    BKKSWG[getValue]
    BSKRAT-->BKKSWG
    BSKRNL-->BKKSWG
    ASKRSK-->BKKSWG
    ASKRUAK-->BKKSWG
    ABKAK-->BKKSWG
    ABKALT-->BKKSWG
    end

    subgraph BitwardenKit.KeychainService
    BKKSS[search]
    BKKSWG-->BKKSS
    end

†diagrams are just for some getters; set and delete and other properties all work similarly from an architectural perspective

github-actions · 2026-03-30T20:35:56Z

Checkmarx One – Scan Summary & Details – 8ef0c8eb-870f-48ef-b154-d1a37dc07507

Great job! No new security vulnerabilities introduced in this pull request

codecov · 2026-03-30T20:42:11Z

Codecov Report

❌ Patch coverage is 96.65236% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.99%. Comparing base (571dd72) to head (dfca6a1).

Files with missing lines	Patch %	Lines
...ared/Core/Platform/Services/ServiceContainer.swift	0.00%	15 Missing ⚠️
...Shared/Core/Auth/Services/KeychainRepository.swift	87.30%	8 Missing ⚠️
...SharedKeychain/SharedKeychainRepositoryTests.swift	96.46%	4 Missing ⚠️
...latform/Models/Domain/Mocks/MockKeychainItem.swift	50.00%	3 Missing ⚠️
...d/Core/Auth/Services/KeychainRepositoryTests.swift	97.94%	3 Missing ⚠️
...eKit/SharedKeychain/SharedKeychainRepository.swift	92.59%	2 Missing ⚠️
...enKit/Core/Platform/Services/KeychainService.swift	80.00%	2 Missing ⚠️
...Core/Platform/Services/KeychainServiceFacade.swift	98.24%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2507      +/-   ##
==========================================
- Coverage   87.07%   85.99%   -1.09%     
==========================================
  Files        1859     2086     +227     
  Lines      164172   178692   +14520     
==========================================
+ Hits       142955   153668   +10713     
- Misses      21217    25024    +3807

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

KatherineInCode · 2026-04-07T17:36:48Z

BitwardenKit/Core/Platform/Models/Domain/Mocks/MockKeychainItem.swift

 extension MockKeychainItem: Equatable {
-    public convenience init(unformattedKey: String) {
+    public convenience init(
+        unformattedKey: String,


Not alphabetized to put optional parameters at the end

KatherineInCode · 2026-04-07T17:48:46Z

BitwardenKit/Core/Platform/Services/KeychainServiceFacade.swift

+    }
+
+    public func getValue(for item: any KeychainItem) async throws -> String {
+        let foundItem = try await keychainService.search(


While this could have gone in the guard, I feel like this is a little easier to read.

github-actions · 2026-04-07T18:48:01Z

Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR consolidates keychain interaction logic from three separate implementations (BitwardenShared.KeychainRepository, AuthenticatorShared.KeychainRepository, and AuthenticatorBridgeKit.SharedKeychainStorage) into a single KeychainServiceFacade in BitwardenKit. The facade correctly preserves all existing keychain behaviors including access control flags, protection levels, the update-then-add race condition prevention pattern, and per-context storage key formatting. Test coverage is thorough, with bespoke mocks replaced by AutoMockable-generated mocks and new Swift Testing tests for the facade itself.

Code Review Details

No findings identified. The refactoring is clean and well-tested.

Security-critical keychain behaviors (access groups, protection levels, access control flags) are correctly preserved through the facade
The KeychainNamespacing enum cleanly models the two different key formatting strategies (app-scoped with prefix vs shared bare keys)
The DefaultKeychainService.search() behavioral change (returning nil instead of throwing for errSecItemNotFound) is correctly handled by the facade's getValue method
Storage key formats are preserved exactly: bwKeyChainStorage for Password Manager, bwaKeychainStorage for Authenticator, and bare unformatted keys for shared keychain items

matt-livefront

This is great, nice job!

matt-livefront · 2026-04-08T15:02:17Z

AuthenticatorBridgeKit/SharedKeychain/SharedKeychainRepository.swift

+
+    // MARK: AuthenticatorKey
+
+    public func deleteAuthenticatorKey() async throws {


⛏️ Looks like this was like this before, but should this be documented? Or should the other methods not be documented since they are documented by the protocol?

matt-livefront · 2026-04-08T16:25:08Z

AuthenticatorBridgeKit/SharedKeychain/SharedKeychainRepositoryTests.swift

+        try await subject.setAccountAutoLogoutTime(date, userId: "1")
+
+        XCTAssertTrue(keychainServiceFacade.setValueCalled)
+        XCTAssertEqual(
+            keychainServiceFacade.setValueReceivedArguments?.item.unformattedKey,
+            SharedKeychainItem.accountAutoLogout(userId: "1").unformattedKey,


🎨 Would it be worth also validating the value that was passed to the facade here (and the other set methods in this file)?

KatherineInCode added 15 commits March 26, 2026 16:09

Getvalue initial

b6b0513

setValue initial

b359f17

More set

acb5d43

deleteValue initial

176d4c6

Handle shared things better

393dabe

Move to enum for namespacing

9e7efde

Adjust tests

e99d2db

Updates to tests

960a9bc

Merge branch 'main' into pm-34009/keychain-service-facade

a777a0a

Adjust prefix location

af40156

Initial passthrough in sharedstorage

338c433

Replace storage with facade

14a23e8

Fix tests

1e500bd

Use shared facade in BWA repository

15a181a

Initial pass on keychain repository

ec529b1

github-actions bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Mar 30, 2026

KatherineInCode changed the title ~~[PM-34009] Create KeychainServiceFacade~~ [PM-34009] chore: Create KeychainServiceFacade Mar 30, 2026

KatherineInCode added the hold This shouldn't be merged yet label Mar 30, 2026

KatherineInCode added 5 commits March 30, 2026 16:04

Make KeychainService AutoMockable

5b1e60d

Fix test

be174ff

Delete storage

2422d52

Bring back SharedKeychainItem

1c1ac16

Merge branch 'main' into pm-34009/keychain-service-facade

15bfb65

github-actions bot added the t:tech-debt Change Type - Tech debt label Mar 30, 2026

KatherineInCode added 3 commits March 31, 2026 12:25

Update comments

519a8c3

Include optional setter for simplicity

ff0b9d0

Fix up shared keychain repository

741162e

KatherineInCode added 17 commits April 2, 2026 12:30

Update tests to Swift Testing

8c2461a

Fix variable name

fa2b3cf

Merge branch 'main' into pm-34009/keychain-service-facade

9989fda

Fix merge weirdness

c7d7e73

Convert KeychainRepository to Automockable

110fbeb

Fix warnings

5042524

Spin Claude on tests

793d71b

Naive convert to Swift Testing

57abacc

Refactor

70add3b

Update

f74b44b

Continue fixing up tests

a97d331

Update tests

5b509f7

Update tests

56aa14d

Use guard

b74fb5a

Localize thing that doesn't need to be non-local

68fef64

Clean warnings

3994525

Fix warning

98aca51

KatherineInCode commented Apr 7, 2026

View reviewed changes

Merge branch 'main' into pm-34009/keychain-service-facade

dfca6a1

KatherineInCode added ai-review-vnext Request a Claude code review using the vNext workflow and removed hold This shouldn't be merged yet labels Apr 7, 2026

KatherineInCode marked this pull request as ready for review April 7, 2026 18:10

KatherineInCode requested review from a team and matt-livefront as code owners April 7, 2026 18:10

github-actions bot added hold This shouldn't be merged yet and removed ai-review-vnext Request a Claude code review using the vNext workflow labels Apr 7, 2026

KatherineInCode added ai-review-vnext Request a Claude code review using the vNext workflow and removed hold This shouldn't be merged yet labels Apr 7, 2026

matt-livefront reviewed Apr 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-34009] chore: Create KeychainServiceFacade#2507

[PM-34009] chore: Create KeychainServiceFacade#2507
KatherineInCode wants to merge 45 commits intomainfrom
pm-34009/keychain-service-facade

KatherineInCode commented Mar 30, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Mar 30, 2026 •

edited

Loading

Uh oh!

codecov bot commented Mar 30, 2026 •

edited

Loading

Uh oh!

KatherineInCode Apr 7, 2026

Uh oh!

KatherineInCode Apr 7, 2026

Uh oh!

github-actions bot commented Apr 7, 2026 •

edited

Loading

Uh oh!

matt-livefront left a comment

Uh oh!

matt-livefront Apr 8, 2026

Uh oh!

matt-livefront Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		// MARK: AuthenticatorKey

		public func deleteAuthenticatorKey() async throws {

Conversation

KatherineInCode commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

Uh oh!

github-actions bot commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job! No new security vulnerabilities introduced in this pull request

Uh oh!

codecov bot commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

KatherineInCode Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

KatherineInCode Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitwarden Claude Code Review

Uh oh!

matt-livefront left a comment

Choose a reason for hiding this comment

Uh oh!

matt-livefront Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

matt-livefront Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KatherineInCode commented Mar 30, 2026 •

edited

Loading

github-actions bot commented Mar 30, 2026 •

edited

Loading

codecov bot commented Mar 30, 2026 •

edited

Loading

github-actions bot commented Apr 7, 2026 •

edited

Loading